Add sequence callback support for animation timelines#3526
Conversation
This implements #2205 - callbacks can now be injected at specific points in an animation sequence. Callbacks support both forward (onEnter) and backward (onLeave) firing for scrubbing support. Usage: ```js animate([ [element, { x: 100 }, { duration: 1 }], [{ onEnter: () => console.log("midpoint!") }, { at: 0.5 }], [element, { y: 200 }, { duration: 1 }], ]) ``` Implementation has minimal overhead: - SequenceCallbackAnimation is lightweight (no DOM ops, just time tracking) - Callbacks are sorted and stored separately from animations - Only fires callbacks when time crosses thresholds https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF
Code ReviewI've reviewed PR #3526 which adds sequence callback support for animation timelines. Overall, this is a well-implemented feature with strong test coverage. Here's my detailed feedback: ✅ Strengths1. Excellent Architecture
2. Comprehensive Test Coverage
3. Good Documentation
|
- Rename onEnter/onLeave to forward/backward for clarity - Remove dedicated SequenceCallbackAnimation class - Use animateSingleValue(0, 1) with onUpdate to track time crossings - Much simpler implementation with same functionality https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF
PR Review: Add sequence callback support for animation timelinesI've thoroughly reviewed this PR. Overall, this is a well-implemented feature with solid test coverage and clear design decisions. Here's my detailed feedback: ✅ Strengths1. Clean API Design
2. Excellent Test Coverage
3. Minimal Performance Overhead
4. Architecture
🔍 Issues FoundCritical: Incorrect Callback Timing LogicLocation: The current implementation recalculates for (const callback of callbacks) {
const prevTime = prevProgress * totalDuration // ❌ WRONG: recalculated each iteration
if (prevTime < callback.time && currentTime >= callback.time) {
callback.forward?.()
} else if (prevTime >= callback.time && currentTime < callback.time) {
callback.backward?.()
}
}Problem: When multiple callbacks exist at different times, Fix: Move return (progress: number) => {
const currentTime = progress * totalDuration
const prevTime = prevProgress * totalDuration // ✅ Calculate once
for (const callback of callbacks) {
if (prevTime < callback.time && currentTime >= callback.time) {
callback.forward?.()
} else if (prevTime >= callback.time && currentTime < callback.time) {
callback.backward?.()
}
}
prevProgress = progress
}This affects both files (lines 29-38 in
|
The WAAPI path should stay minimal and not pull in animateSingleValue. https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF
Just destructure animationDefinitions directly from createAnimationsFromSequence instead of wrapping it. https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF
PR Review: Add sequence callback support for animation timelinesSummaryThis PR adds callback support to animation sequences, allowing developers to execute code at specific timeline points. The implementation is clean and well-tested with comprehensive coverage. ✅ Code Quality & Best PracticesStrengths:
Minor suggestions:
🐛 Potential Issues1. Edge case: Multiple callbacks at the same time 2. Callback execution during seeks ( 3. Memory leak potential 4. WAAPI implementation (
⚡ Performance ConsiderationsExcellent:
Suggestion: // Early exit when we've passed all callbacks in forward direction
if (currentTime >= callbacks[callbacks.length - 1].time &&
prevTime >= callbacks[callbacks.length - 1].time) {
prevProgress = progress
return
}🔒 Security ConcernsNo security issues identified. Callbacks are user-provided functions executed in the user's context, which is expected behavior. 🧪 Test CoverageExcellent coverage with tests for:
Missing test cases:
📝 DocumentationThe PR description is excellent. Consider adding:
Code-Specific Commentssequence.ts:59-66 - Consider adding a comment explaining why linear ease is used: // Use linear ease to ensure onUpdate receives uniform progress values
// for accurate time threshold detectioncreate.ts:420 - Nice that callbacks are sorted! Consider adding a comment explaining why: // Sort callbacks by time for deterministic execution order
// and potential future optimization (early exit)ConclusionThis is a solid implementation that follows the codebase's high standards. The feature is well-designed, properly tested, and adds valuable functionality. The main suggestions are around edge case documentation and minor optimizations. Recommendation: Approve with minor suggestions ✅ The identified issues are low-severity and mostly around documentation/edge cases rather than fundamental problems. Nice work! |
Code Review: Sequence Callback SupportThank you for this PR! The implementation of callback support for animation sequences is well-designed and thoughtfully executed. Strengths1. Clean Architecture
2. Comprehensive Test Coverage
3. Smart Timing Logic
Potential Issues1. Critical: Callback Firing on Repeated Time Sets (HIGH PRIORITY)Location: packages/framer-motion/src/animation/animate/sequence.ts:31-37 Problem: The implementation does not explicitly track which callbacks have already fired. If progress is set externally via scrubbing, the same callback could fire multiple times during forward playback if the progress crosses the threshold multiple times. Recommendation: Consider tracking which callbacks have fired in each direction using a Set. 2. WAAPI Implementation Missing Callback Support (HIGH PRIORITY)Location: packages/framer-motion/src/animation/animators/waapi/animate-sequence.ts The WAAPI version only extracts animationDefinitions but completely ignores callbacks. This creates an inconsistent API where callbacks work in some contexts but silently fail in others. Recommendation: Either add callback support to WAAPI, log a warning when callbacks are present but ignored, or document this limitation clearly. 3. Type Safety Issue in isCallbackSegmentLocation: packages/framer-motion/src/animation/sequence/create.ts:465-477 The heuristic could incorrectly identify animation segments as callbacks if someone passes forward: 100 trying to animate a forward property. The check for duration/ease helps but is not foolproof. Recommendation: Make callbacks more explicit with a unique property, class constructor, or Symbol key for type discrimination. 4. Performance: Linear Search on Every UpdateLocation: packages/framer-motion/src/animation/animate/sequence.ts:28 On every animation frame, this loops through ALL callbacks even if none are near the current time. For sequences with many callbacks this could impact performance. Recommendation: Implement binary search or maintain a pointer to the next callback to only check callbacks near the current position. 5. Edge Case: Callbacks at Time 0If a callback is positioned at time 0, it might fire immediately when the animation starts. This might be unexpected behavior. Recommendation: Add a test case and clarify documentation on whether time-0 callbacks fire on animation start. Code Style and Best PracticesMinor Issues:
Test Coverage AssessmentExcellent coverage of timing resolution, but missing:
Recommendations SummaryMust Fix before merge:
Should Fix: Nice to Have: Overall AssessmentThis is a high-quality PR with solid implementation and thorough testing. The main concern is the missing WAAPI support and the type discrimination heuristic. With those addressed, this feature will be a valuable addition to the animation API. Score: 8/10 - Would be 9.5/10 after addressing the WAAPI issue and type safety concern. Let me know if you would like me to provide code snippets for any of the suggested fixes! |
Return ResolvedAnimationDefinitions directly instead of a wrapper object. Callbacks are passed via an optional out-parameter that only animateSequence uses. WAAPI path and tests are unaffected. https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF
Code Review SummaryThank you for this well-structured PR! The implementation of sequence callbacks is clean and follows good practices. Below is my detailed feedback: ✅ StrengthsCode Quality
Implementation Details
🔍 Issues & Concerns1. CRITICAL: WAAPI Implementation Missing Callback SupportThe WAAPI version of // waapi/animate-sequence.ts - Line 12
createAnimationsFromSequence(definition, options).forEach(
({ keyframes, transition }, element: Element) => {
animations.push(...animateElements(element, keyframes, transition))
}
)
// ❌ Missing: No callbacks array, no callback animation addedImpact: Callbacks will silently not work when using WAAPI animations, leading to confusing behavior differences between animation backends. Recommendation: Update the WAAPI implementation to match the main implementation, or document why callbacks are not supported in WAAPI mode. 2. Edge Case: Empty Sequence with Only Callbacks// packages/framer-motion/src/animation/animate/sequence.ts:67-71
const firstTransition = animationDefinitions.values().next().value?.transition
const totalDuration = firstTransition
? (Object.values(firstTransition)[0] as any)?.duration ?? 0
: 0If a sequence contains only callbacks and no animations,
Test case that would expose this: const sequence = [
[{ forward: () => console.log('1') }, { at: 0 }],
[{ forward: () => console.log('2') }, { at: 1 }],
]
// Expected: callbacks fire at 0s and 1s
// Actual: both fire immediately at 0sRecommendation: Either:
3. Type Safety: Unsafe
|
…, fix timing assertion - Rename forward/backward to enter/leave across types, create, and sequence - Add 4 integration tests: scrubbing thresholds, complete(), cancel() without scrub, cancel() after scrub - Fix unit test expectation for trailing keyframe offset in callback timing test https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF
Pull Request Review: Sequence Callback SupportThank you for this well-implemented feature! I've completed a thorough review of the PR and have the following feedback: ✅ Strengths
🔍 Issues FoundCritical Issues1. Missing WAAPI ImplementationLocation: The WAAPI version of Issue: The function doesn't pass an Recommendation: Either implement callback support for WAAPI (preferred), document that callbacks require JS animations, or add a runtime warning when callbacks are used with WAAPI. 2. Edge Case: Empty SequencesLocation: If a sequence contains only callbacks and no animations, Recommendation: Track 3. Callback Detection Logic Has False PositivesLocation: The
Recommendation: The detection should check that the callback object only contains Performance Considerations4. Linear Search on Every Frame UpdateLocation: This loops through ALL callbacks on every frame update, even after they've fired. For sequences with many callbacks, this creates unnecessary overhead. Impact: 10 callbacks = ~600 comparisons per second at 60fps; 100 callbacks = 6000 comparisons per second Recommendation: Track an active range index and only check callbacks near the current time. This reduces complexity from O(n) per frame to O(callbacks_crossed) per frame. Code Quality5. Use of
|
- Rename leave → exit across types, create, sequence, and tests - Replace fragile duration lookup with SequenceCallbackData out-parameter - Simplify isCallbackSegment type guard - Use optional chaining for callbackData in create.ts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review for PR #3526: Add sequence callback support for animation timelinesI've completed a thorough review of this PR. Overall, this is a well-designed and thoughtfully implemented feature with excellent test coverage. Here's my detailed feedback: ✅ Strengths1. Excellent API Design
2. Robust Implementation
3. Comprehensive Test Coverage
4. Good Documentation
|
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Add sequence callback support for animation timelinesThis is a well-implemented feature that addresses issue #2205. The implementation is clean, efficient, and follows the repository's conventions. Here's my detailed review: ✅ Strengths1. Excellent API Design
2. Smart Implementation Strategy
3. Comprehensive Test Coverage
4. Clean Type Safety
🔍 Potential Issues & Questions1. Callback Timing Edge Case (Minor)In if (prevTime < callback.time && currentTime >= callback.time) {
callback.enter?.()
} else if (prevTime >= callback.time && currentTime < callback.time) {
callback.exit?.()
}Issue: If Test case to consider: const animation = animate([
[{ enter: () => console.log('enter'), exit: () => console.log('exit') }, { at: 0 }],
[element, { x: 100 }, { duration: 1 }],
])
animation.pause()
animation.time = 0 // enter fires (0 < 0 is false, but 0 >= 0 is true)
animation.time = 0.5
animation.time = 0 // exit won't fire (0.5 >= 0 and 0 < 0)Suggested consideration: The edge case of callbacks at 2. Multiple Callbacks at Same Time (Question)The code sorts callbacks and iterates through all of them. What happens if multiple callbacks are defined at the exact same time? animate([
[element, { x: 100 }, { duration: 1 }],
[{ enter: () => console.log('first') }, { at: 0.5 }],
[{ enter: () => console.log('second') }, { at: 0.5 }],
])Current behavior: Both callbacks will fire (in array order after sorting). This seems correct, but it's worth documenting or adding a test case. 3. Memory Leak Prevention (Question)When Verification: Check if 4. WAAPI Path Limitation (Informational)The PR description mentions "The WAAPI path should stay minimal" - this means callbacks won't work with WAAPI animations. This is fine, but consider:
5.
|
Reorder describe blocks so callback tests run before the buggy "non-animatable color" test that corrupts frame loop state. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Add sequence callback support for animation timelinesI've reviewed this PR and overall the implementation is solid. Here's my detailed feedback: ✅ Strengths
|
Summary
This PR adds support for callbacks in animation sequences, allowing developers to execute code at specific points during a sequence timeline. Callbacks can be positioned at absolute times, relative offsets, or referenced by labels, and support both forward playback (onEnter) and backward scrubbing (onLeave).
Key Changes
New
SequenceCallbackAnimationclass (motion-dom): A lightweight animation implementation that fires callbacks when the timeline crosses specific time points. It handles both forward playback and backward scrubbing scenarios.Callback segment support: Extended
AnimationSequencetype to support callback segments as[{ onEnter?, onLeave? }, { at? }]tuples, allowing callbacks to be interspersed with animations.Enhanced
createAnimationsFromSequence:CreateAnimationsResultobject containinganimationDefinitions,callbacks, andtotalDurationatpositioning logic as animations (absolute, relative+/-,<for previous, or label references)Integration with animation sequences: Updated both
animateSequencefunctions (inframer-motionandwaapi) to instantiate and includeSequenceCallbackAnimationin the animation group when callbacks are present.Comprehensive test coverage: Added 10+ tests covering callback extraction, timing resolution, multiple callbacks, label-based positioning, and interaction with animation timing.
Implementation Details
SequenceCallbackAnimationuses simple time comparison logic with minimal overheadonLeavecallbacks, enabling interactive timeline scrubbing scenarioshttps://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF